-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Optimize BuildFQName function #1665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Jan-Otto Kröpke <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful, thanks!
I'm skeptical that the extra overhead of using strings.Builder makes sense for such a short string. Indeed, the strings.Builder approach is barely any different here. But if we're going for optimization, sometimes the simplest and most explicit code is the fastest, to let the compiler optimize it more efficiently. A simple string concatenation is about twice as fast as the strings.Builder approach. func BuildFQNameConcat(namespace, subsystem, name string) string {
if name == "" {
return ""
}
switch {
case namespace != "" && subsystem != "":
return namespace + "_" + subsystem + "_" + name
case namespace != "":
return namespace + "_" + name
case subsystem != "":
return subsystem + "_" + name
}
return name
} Benchmarking all three approaches:
Just for the record, the second benchmark of each implementation is moot, since that passes an empty if name == "" {
return ""
} |
No clue, why my contribution is even slower as the original one. The only different I saw is that I used As I know, the performance of string concat depends on memory speed, since each string concat operation copy memory. The compiler optimization is limited on that region. source code: It could be possible that your memory speed on your 11th intel cpu is much higher than on my old 4th gen which result into much better results on string concats.
If there is short-path, why the times are so different? It make no sense to me.
|
I've used strings.Builder before, and it certainly has its uses. The fact that it's internally using a backing byte slice and calling I think the reason why the simple string concatenation approach is significantly faster is that the compiler is inlining that code. If I force it to not inline with
The inlining of that function explains why the empty-name fast path is ... faster. Since you suspect that it is dependent on CPU generation and memory speed, I also ran the benchmark on a quite old Intel Core i3 (without the
In summary, the strings.Builder approach is somewhat faster than the original strings.Join approach, but the simple concatenation (which the compiler sees fit to inline) is faster still. |
At least, this results matches my results. I had never proposed strings.Builder, if it would be slower than the original one. Good to know that string concat is faster in specific situations. Thats something what I learned today. |
The BuildFQName function is used on many exporters multiple times.
The overall benefit might be quite low, but the I'm happy to contribute an optimization.
strings.Builder
is available since go 1.10 and should me the current requirements.Go test commands:
Benchmark File: